-
-
Notifications
You must be signed in to change notification settings - Fork 325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unique Identifiers for POST Requests #98
Conversation
…where a unique identifier is required as part of each request such as when creating new users where email must be unique
…st rather than per connection. Added code to correct content length in request builder
…_from function in order to ensure Node <6 compatibility
Thanks for your contribution. I only have one request, and it is that the id-replicating logic should be configurable, not enabled by default. |
No problem, it's been fun to work on. I have replaced uuid with hyperid and made id-replacement configurable as you asked. I've updated the README to reflect the latter change. |
@@ -60,7 +60,8 @@ function requestBuilder (defaults) { | |||
} | |||
|
|||
if (bodyBuf && bodyBuf.length > 0) { | |||
headers['Content-Length'] = '' + bodyBuf.length | |||
const uuidCount = (bodyBuf.toString().match(/\[<uuid>\]/g) || []).length | |||
headers['Content-Length'] = `${bodyBuf.length + (uuidCount * 28)}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maths relating to this content-length is wrong, I think.
content-length = bodyBuf.length - ('<uuid>'.length * uuidCount) + (uuidCount * 28)
Because the bodyBuf string will remove the '' tokens to replace them with the uuid's
Also is an uuid not 16 bytes in length?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will absolutely do that, I think in my haste to respond to the earlier comment I forgot to go back over the maths. I'd gotten the earlier figure by comparing the content length with the tag and with the UUID, but I will look into this and add a test as you suggested below. Should have time to work on this tonight or tomorrow.
If you wouldn't mind, could you add a basic test to validate this works as expected? Thank you for this PR, it looks awesome 👍 |
@@ -90,6 +90,8 @@ Available options: | |||
Don't render the progress bar. default: false. | |||
-l/--latency | |||
Print all the latency data. default: false. | |||
-I/--idReplacement | |||
Enable replacement of [<uuid>] with a randomly generated unique identifier within the request body. default: false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call it <id>
, or :id:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to [<id>]
@@ -1,3 +1,4 @@ | |||
const uuid = require('hyperid')() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a uuid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to hyperid
@@ -28,7 +29,7 @@ RequestIterator.prototype.nextRequestBuffer = function () { | |||
|
|||
RequestIterator.prototype.move = function () { | |||
// get the current buffer and proceed to next request | |||
const ret = this.currentRequest.requestBuffer | |||
const ret = new Buffer(this.currentRequest.requestBuffer.toString().replace(/\[<uuid>\]/g, uuid())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is where we lose speed. Can we do this change using a function passed in as an option? In this way we could support more scenarios that just :id:
.
const ret = this.currentRequest.requestBuffer | ||
const ret = this.reqDefaults.idReplacement | ||
? new Buffer(this.currentRequest.requestBuffer.toString().replace(/\[<uuid>\]/g, uuid())) | ||
: this.currentRequest.requestBuffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we change this logic so that this mangling logic is an opt-in function?
…anged instance name of hyperid to hyperid to reflect that it is not a uuid
…arlier versions of node
const contentLengthRegex = /\[<contentLength>\]/ | ||
const reqStr = ret.toString().replace(/\[<id>\]/g, hyperid()) | ||
const bodyLength = reqStr.split(contentLengthRegex)[1].trim().length | ||
ret = new Buffer(reqStr.replace(contentLengthRegex, `${bodyLength}`)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All those steps are going to be really slow, and we are running it twice.
Isn't there a chance to have this logic into the requestBuilder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only problem with moving it into requestBuilder is that the length of the replacement value on line 37 will vary. As you will know, hyperid generates IDs made up of a base64 string plus an integer, the integer will grow in length as it gets larger, which prevents setting content length until all replacements have been carried out.
This would not be a problem if we used UUIDs as they would be a fixed size, of course then there is a penalty for constantly regenerating UUIDs. I could fork and submit a change to the implementation of hyperid to pad the iteration section out to a fixed width but I don't know how expensive that would be in terms of the performance of hyperid. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's try adding a pad to hyperid. It does not grow indefinitely, but up to https://github.com/mcollina/hyperid/blob/master/hyperid.js#L4. Maybe as an option when we create the hyperid instance.
Good progress! I think we are nearly there! @thekemkid what do you think? |
I'm loving this work 👍This feature will be fairly complex to understand purely based on the docs, would an example be appropriate? Other than that, I don't have much to add, and I don't want to step on your toes @darArch 😄 So for now I'm just keeping an eye on how this pr is progressing, but if you need any help, I'm happy to jump in and give you the support you need where I can! :) |
Also, make sure you add your name to the |
… back to request builder and simplified request iterator. Updated tests as appropriate and tested manually with personal site. Added name to contributors section :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing. LGTM
@darArch would you like to help maintaining/developing autocannon? |
@mcollina I would definitely be interested. It's been great fun to use and work with/on. |
@mcollina @thekemkid The Travis CI build failed when Node v6 failed to install with NVM, would you be able to retrigger the build? |
@thekemkid Should I add an example body before and after tag replacement to the README? |
@darArch an example will be better, I would like to keep the README thin. |
🎉Great to hear you're happy to join 🎉 I'm rebuilding now, and defer to @mcollina on the example 👍 |
@darArch just invited you to the repo! |
…dReplacement to default options in run.js, updated README
Not sure what's going on with the Travis CI builds, but I just added an example in samples/using-id-replacement and linked to it in the README. |
NVM on travis seems to be having some trouble right now, lets leave it be for the evening, and retrigger the build in the morning. If its something deeper, I'll investigate further over the weekend. |
I retriggered the Travis build which has now passed. I don't see any option to retrigger the Appveyor build though, @mcollina would you be able to? |
All is good now. @thekemkid would you mind doing a release? (Also check if native-hdr-histogram is all good). |
Problem
In order to automate soak testing of POST endpoints we need to send some unique identifier as part of the request. For example, when registering a new user for a website, we require the email address used to be unique.
Solution
Alteration of AutoCannon to recognise a specific tag,
[<uuid>]
, within the request body and replace it with a new version 4 UUID. UUID generation is handled by the uuid package. In order to ensure per request rather than per connection unique identifiers, the tag is replaced as part of the Request Iterator rather than Request Builder. The Request Builder is also modified to add 28 bytes per usage of the tag to theContent-Length
header to account for the replacement.